gh-91851: Micro optimizations for Fraction's arithmetic #25518
Conversation
|
This PR is stale because it has been open for 30 days with no activity. |
skirpichev
changed the title
github-actions
bot
removed
the
stale
|
@hauntsaninja, actually there were benchmarks (second commit message). Now copied to the pr's body, thanks. |
@skirpichev @hauntsaninja The PR indeed makes addition of Fraction and int slower. The regression is in this https://github.com/python/cpython/pull/25518/files#diff-f561eb7eb97f832b2698837f52c2c2cf23bdb0b31c69cf1f6aaa560280993316R398 change, which forces the The optimizations in |
|
Also here https://github.com/python/cpython/pull/25518/files#diff-f561eb7eb97f832b2698837f52c2c2cf23bdb0b31c69cf1f6aaa560280993316R414 there is a regression for the case |
|
@eendebakpt, arithmetic optimization assume that the hot path is
|
|
If you create a separate PR for round, ceil and floor, I can merge it since that's uncontroversial :-) |
|
@hauntsaninja , see #100791 |
Adapted from 046c84e8f9 That makes arithmetics for small components just as fast as before #24779, except for a mixed case (e.g. Fraction + int): On master: $ ./python -m timeit -s 'from fractions import Fraction as F' \ -s 'a = F(7, 8)' -s 'b = F(3, 4)' 'a + b' 20000 loops, best of 5: 12.4 usec per loop $ ./python -m timeit -s 'from fractions import Fraction as F' \ -s 'a = F(7, 8)' -s 'b = 3' 'a + b' 20000 loops, best of 5: 10.3 usec per loop $ ./python -m timeit -s 'from fractions import Fraction as F' \ -s 'a = 3' -s 'b = F(3, 4)' 'a + b' 20000 loops, best of 5: 12.6 usec per loop With the patch: $ ./python -m timeit -s 'from fractions import Fraction as F' \ -s 'a = F(7, 8)' -s 'b = F(3, 4)' 'a + b' 20000 loops, best of 5: 9.98 usec per loop $ ./python -m timeit -s 'from fractions import Fraction as F' \ -s 'a = F(7, 8)' -s 'b = 3' 'a + b' 20000 loops, best of 5: 15.6 usec per loop $ ./python -m timeit -s 'from fractions import Fraction as F' \ -s 'a = 3' -s 'b = F(3, 4)' 'a + b' 20000 loops, best of 5: 16.6 usec per loop
|
Thanks for splitting off the other PR and rebasing this one. I don't have good enough insight into fractions to know if this tradeoff is worth it, e.g. if Fraction + int is as common as Fraction + Fraction, this is not a win. |
|
The change to use properties for |
|
The changes LGTM - I find the new code to be cleaner (as well as fast for the |
The commit is 400adb0 Such a change would impact subclasses of Fraction that directly set '._numerator' . I am not aware of such subclasses, but they can exist. (to be clear: this PR is fine) |
|
Thanks for the PR! And yes, definitely nice that monomorphic_operator is now more monomorphic |
|
Thanks to all for reviews.
@eendebakpt You can do this as long as you respect constraints on these private attributes. I.e. they are essentially numerator/denominator properties of the numbers.Rational ("The numerator and denominator values should be instances of Integral and should be in lowest terms with denominator positive."). If you ignore this - some methods of the Fraction class will be broken anyway, starting from the I hardly can imagine benefits of subclassing the Fraction. E.g. if you want to keep components in an unnormalized form - most arithmetic methods should be revised. |
This apply some suggestions from the #24779 review.
I've tested also direct imports (e.g.
from math import gcd), suggested by @tim-one, but the difference doesn't seems to be noticeable.Resolves #91851
Benchmarks (taken from the second commit msg):
That makes arithmetics for small components just as fast
as before #24779, except for a mixed case (e.g.
Fraction + int):
On master:
With the patch: